Fixes for kci #2904
Fixes for kci #2904
Conversation
|
Now it works (with example toml config): |
| @@ -180,6 +180,7 @@ def group(self, *args, cls=None, **kwargs): | |||
|
|
|||
| @click.group(cls=KciGroup) | |||
| @Args.settings | |||
| # @Args.config # Removed to allow subcommands to receive -c/--yaml-config | |||
There was a problem hiding this comment.
I was able to use -c successfully without this change.
Could you provide more context on how does this change fix -c logic?
There was a problem hiding this comment.
If i reverse
./kci job generate 684ca438dba9af5ad63511e5 --runtime lava-collabora -c ../kernelci-pipeline/config/
Traceback (most recent call last):
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 35, in <module>
main()
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 31, in main
kci() # pylint: disable=no-value-for-parameter
^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1654, in invoke
super().invoke(ctx)
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/decorators.py", line 26, in new_func
return f(get_current_context(), *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: kci() got an unexpected keyword argument 'config'
and as global wont work either:
./kci -c ../kernelci-pipeline/config/ job generate 684ca438dba9af5ad63511e5 --runtime lava-collabora
Traceback (most recent call last):
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 35, in <module>
main()
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 31, in main
kci() # pylint: disable=no-value-for-parameter
^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1654, in invoke
super().invoke(ctx)
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/decorators.py", line 26, in new_func
return f(get_current_context(), *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: kci() got an unexpected keyword argument 'config'
There was a problem hiding this comment.
I see and now it's even more confusing to me - here's what works for me:
./kci job generate -c ../kernelci-pipeline/config/ 684a6749dba9af5ad62e4a68
so:
./kci $command $subcommand $global_options $subcommand_argument
There was a problem hiding this comment.
diff --git a/kernelci/cli/__init__.py b/kernelci/cli/__init__.py
index a3c02b58..a9d405a9 100644
--- a/kernelci/cli/__init__.py
+++ b/kernelci/cli/__init__.py
@@ -180,7 +180,7 @@ class KciGroup(click.core.Group):
@click.group(cls=KciGroup)
@Args.settings
-# @Args.config # Removed to allow subcommands to receive -c/--yaml-config
[email protected]
@click.pass_context
def kci(ctx, settings):
"""Entry point for the kci command line tool"""
If i put it back it doesnt work for me:
./kci job generate -c ../kernelci-pipeline/config/ 684a6749dba9af5ad62e4a68
Traceback (most recent call last):
File "/home/nuclear/Documents/kernelci-core/./kci", line 35, in <module>
main()
~~~~^^
File "/home/nuclear/Documents/kernelci-core/./kci", line 31, in main
kci() # pylint: disable=no-value-for-parameter
~~~^^
File "/usr/lib/python3/dist-packages/click/core.py", line 1161, in __call__
return self.main(*args, **kwargs)
~~~~~~~~~^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/click/core.py", line 1082, in main
rv = self.invoke(ctx)
File "/usr/lib/python3/dist-packages/click/core.py", line 1694, in invoke
super().invoke(ctx)
~~~~~~~~~~~~~~^^^^^
File "/usr/lib/python3/dist-packages/click/core.py", line 1443, in invoke
return ctx.invoke(self.callback, **ctx.params)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/click/core.py", line 788, in invoke
return __callback(*args, **kwargs)
File "/usr/lib/python3/dist-packages/click/decorators.py", line 33, in new_func
return f(get_current_context(), *args, **kwargs)
TypeError: kci() got an unexpected keyword argument 'config'
| @@ -75,13 +75,22 @@ class Runtime(abc.ABC): | |||
| TEMPLATES = ['config/runtime', '/etc/kernelci/runtime'] | |||
|
|
|||
| # pylint: disable=unused-argument | |||
| def __init__(self, config, user=None, token=None): | |||
| def __init__(self, config, user=None, token=None, custom_template_dir=None): | |||
There was a problem hiding this comment.
Please provide more context on how this feature is intended to be used and what key benefits do you see by enabling additional runtime config directory tree.
There was a problem hiding this comment.
By default jinja will search templates in kernelci-core/config/runtime only, it wont search in pipeline config.
This is what will happen:
Traceback (most recent call last):
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 35, in <module>
main()
File "/home/nuclearcat/Documents/TEST/kernelci-core/./kci", line 31, in main
kci() # pylint: disable=no-value-for-parameter
^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/Documents/TEST/kernelci-core/kernelci/cli/__init__.py", line 150, in invoke
super().invoke(ctx)
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/Documents/TEST/kernelci-core/kernelci/cli/__init__.py", line 78, in call
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/Documents/TEST/kernelci-core/kernelci/cli/job.py", line 123, in generate
job_data = runtime.generate(job, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/Documents/TEST/kernelci-core/kernelci/runtime/lava.py", line 327, in generate
template = self._get_template(job.config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/Documents/TEST/kernelci-core/kernelci/runtime/__init__.py", line 106, in _get_template
return jinja2_env.get_template(job_config.template)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/jinja2/environment.py", line 1016, in get_template
return self._load_template(name, globals)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/jinja2/environment.py", line 975, in _load_template
template = self.loader.load(self, name, self.make_globals(globals))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/nuclearcat/.local/lib/python3.12/site-packages/jinja2/loaders.py", line 607, in load
raise TemplateNotFound(name)
jinja2.exceptions.TemplateNotFound: generic.jinja2
In job definition we have baseline.jinja2 for this particular job, which need to be retrieved from custom config directory (../kernelci-pipeline/config), or we can try to do a mess and merge configs from pipeline to core manually, which is IMHO very wrong.
There was a problem hiding this comment.
You're right, it's already messy enough - I don't have an answer on how to proceed with it yet
kernelci/cli/job.py
Outdated
| def _get_runtime(runtime, config, secrets): | ||
| configs = kernelci.config.load(config) | ||
| runtime_config = configs['runtimes'][runtime] | ||
| runtime = kernelci.runtime.get_runtime( | ||
| runtime_config, token=secrets.api.runtime_token, | ||
| custom_template_dir=config[0] if config else None | ||
| ) | ||
| return runtime |
There was a problem hiding this comment.
This change reverts all the checks you introduced earlier. I believe this wasn't intended (maybe rebase conflict resolution hiccup?) - please help me understand it if that's not the case.
There was a problem hiding this comment.
linter complained there is too many branches in main function. I just moved part of logic from kbuild to separate function, and decided to remove these checks, and to work later to make them more elegant and consistent.
There was a problem hiding this comment.
- You added useful checks in "Add proper error if runtime not found" commit
- You received linter complaint
- You extracted part of the logic to a separate function and reverted your initial change
In this case I'd say it's reasonable to ignore linter hints - there is no strict rule that codebase has to follow such suggestions to the letter.
I also disagree that there will be an opportunity
to work later to make them more elegant and consistent
Project's development history so far strongly suggests quite the opposite of it
|
Full steps to reproduce: |
nuclearcat
force-pushed
the
fixes-for-kci
branch
2 times, most recently
from
5735413 to
c7867ab
Compare
There was a problem hiding this comment.
@nuclearcat I understand that my comments to this series might seem like a "patch sculpting" but that's not my intention - I try to work out with you a consistent and clear way of addressing issues you noticed and attempted to fix in the code base.
If this is not what you expect from a reviewer for your patch series I'm fine to hand it over and revert change requests.
| @@ -180,6 +180,7 @@ def group(self, *args, cls=None, **kwargs): | |||
|
|
|||
| @click.group(cls=KciGroup) | |||
| @Args.settings | |||
| # @Args.config # Removed to allow subcommands to receive -c/--yaml-config | |||
There was a problem hiding this comment.
I see and now it's even more confusing to me - here's what works for me:
./kci job generate -c ../kernelci-pipeline/config/ 684a6749dba9af5ad62e4a68
so:
./kci $command $subcommand $global_options $subcommand_argument
| @@ -75,13 +75,22 @@ class Runtime(abc.ABC): | |||
| TEMPLATES = ['config/runtime', '/etc/kernelci/runtime'] | |||
|
|
|||
| # pylint: disable=unused-argument | |||
| def __init__(self, config, user=None, token=None): | |||
| def __init__(self, config, user=None, token=None, custom_template_dir=None): | |||
There was a problem hiding this comment.
You're right, it's already messy enough - I don't have an answer on how to proceed with it yet
kernelci/cli/job.py
Outdated
| def _get_runtime(runtime, config, secrets): | ||
| configs = kernelci.config.load(config) | ||
| runtime_config = configs['runtimes'][runtime] | ||
| runtime = kernelci.runtime.get_runtime( | ||
| runtime_config, token=secrets.api.runtime_token, | ||
| custom_template_dir=config[0] if config else None | ||
| ) | ||
| return runtime |
There was a problem hiding this comment.
- You added useful checks in "Add proper error if runtime not found" commit
- You received linter complaint
- You extracted part of the logic to a separate function and reverted your initial change
In this case I'd say it's reasonable to ignore linter hints - there is no strict rule that codebase has to follow such suggestions to the letter.
I also disagree that there will be an opportunity
to work later to make them more elegant and consistent
Project's development history so far strongly suggests quite the opposite of it
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Also in example correct config for current production. Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat
force-pushed
the
fixes-for-kci
branch
from
c7867ab to
bde3248
Compare
There was a problem hiding this comment.
@nuclearcat Thanks for addressing most of my concerns. I understand the need for "cutting corners" to make the Users happy. I have no intention of blocking this patch series anymore.
This is various fixes for kci usability reported by @broonie
Even with included patches, still major issue remains, config of pipeline need to be "added" to kernelci-config for job definitions and runtimes.
And runtime templates from kernelci-pipeline need to be added to kernelci-core/config/runtime.
I need to figure out how to do that user-friendly way
Fixes: #2903